-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Consolidate importer spilling code V2 #72744
Consolidate importer spilling code V2 #72744
Conversation
* Add tests * Fix losing GLOB_REF on the LHS The comment states we don't need it, which is incorrect. Diffs are improvements because we block forward substitution of calls into "ASG(BLK(ADDR(LCL_VAR<field>, ...)))", which allows morph to leave the "can be replaced with its field" local alone. * Prospective fix Spill "glob refs" on stores to "aliased" locals. * Delete now-not-necessary code * Fix up asserts * Clean out '(unsigned)CHECK_SPILL_ALL/NONE' casts * Don't manually spill for 'st[s]fld' * Revert 'Clean out '(unsigned)CHECK_SPILL_ALL/NONE' casts'
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
93a293b
to
52871c4
Compare
The mistake in logic was that the only trees which could modify unaliased locals are assignments, which is not true, calls can do that as well. One day we will move the return buffer handling out of importer, but until then, special handling is required. An alternative fix would have been to bring back the explicit "impSpillLclRefs" to "stloc/starg" code, but that would contradict the overall goal of consolidating the spilling logic.
52871c4
to
b537705
Compare
@dotnet/jit-contrib |
/azp run runtime-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
R2R failures are #72822 |
Unfortunately we have several silent bad codegen examples in the latest Fuzzlyn run that look caused or exposed by this PR: https://dnceng.visualstudio.com/public/_build/results?buildId=1914436&view=ms.vss-build-web.run-extensions-tab (ignore linux-arm examples) At this point I don't think we should take this for 7.0.0 since we may have a bug tail to work through. I will revert it, but please feel free to resubmit it and look into those issues. |
This reverts commit cea17b2.
* Consolidate importer spilling code (dotnet#72291) * Add tests * Fix losing GLOB_REF on the LHS The comment states we don't need it, which is incorrect. Diffs are improvements because we block forward substitution of calls into "ASG(BLK(ADDR(LCL_VAR<field>, ...)))", which allows morph to leave the "can be replaced with its field" local alone. * Prospective fix Spill "glob refs" on stores to "aliased" locals. * Delete now-not-necessary code * Fix up asserts * Clean out '(unsigned)CHECK_SPILL_ALL/NONE' casts * Don't manually spill for 'st[s]fld' * Revert 'Clean out '(unsigned)CHECK_SPILL_ALL/NONE' casts' * Fix assignments done via return buffers The mistake in logic was that the only trees which could modify unaliased locals are assignments, which is not true, calls can do that as well. One day we will move the return buffer handling out of importer, but until then, special handling is required. An alternative fix would have been to bring back the explicit "impSpillLclRefs" to "stloc/starg" code, but that would contradict the overall goal of consolidating the spilling logic.
Same as #72291, but handles calls with return buffers correctly (second commit).
Fixes #72133.